-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
initial commit of the low-code nft marketplace #8
initial commit of the low-code nft marketplace #8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of it looks pretty good, but there are a few things that need to be changed
- I don't think we should have a duplicate copy of the CIS2-multi contract. We should work to get the one in rust-smart-contracts modified if practical.
- There is a bunch of duplication for generic stuff for interacting with CIS2 contracts. That should be moved to the node SDK and the duplication should be removed.
- The typescript projects should have linting as well.
- Cargo.lock files should be committed for contracts.
low-code-nft-marketplace/mint-ui/src/models/Cis2Deserializer.ts
Outdated
Show resolved
Hide resolved
…of official CIS2-Multi contract
… into low-code-nft-marketplace
@abizjak : We have done our best efforts to fix the issues and to address the comments. Kindly review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking OK now.
However I am wondering what level of scrutiny we should apply to the contracts. I have not done a full audit, but I'm thinking that if we are going to promote this we should do a thorough analysis of the marketplace contract to make sure we don't have problems.
@bogacyigitbasi Can you answer what the intended scope of this is?
I get errors running the
|
Some suggestions for the future (in new PRs if we decide to incorporate it) once we get the first version merged and we discuss future plans:
|
Suggestion: We need to explain what "primary seller royalties" are. It is not the creator but the person that first sells the NFT on this marketplace that can set the royalties. Maybe that is part of the developer documentation then that is fair enough. It would be super nice if there is an explanation at the front end explaining how the royalties work in this marketplace and how the sales price is calculated (aka what the seller, primary seller, and the marketplace get from a sold NFT). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project is a great addition to our tools. Well written and it can serve as a good start for people to create their own projects. There went a lot of work in this PR and this is appreciated. So depending on how polished we want to get it, I am fine if we decide to address some of the remaining comments in future PRs to get this massive PR merged.
I had some hiccups with the pinata/front-end flow so (the keys that I just generated didn’t work reliably but I got some borrowed keys that worked). Since users can skip that step, we are good to go for the first version. If we get some more feedback about the pinata flow, we can improve on it to make it easier for people to get through e.g. by improving the documentation how and which types of keys they need to generate.
- Does the user have to wait after creating the pinata key (until it is activated)?
- Can I upload the same picture (the ipfs hash has not changed) in the flow several times?
low-code-nft-marketplace/market-ui/src/components/MarketplaceTokensList.tsx
Show resolved
Hide resolved
Hey @DOBEN , here are some explanations about how it works/why?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done. This project is a valuable contribution to our dApp examples. Thanks for your continued effort in working on it.
Purpose
_Describe the purpose of the pull request, link to issue describing the problem, etc.
Changes
_Describe the changes that were needed.
Depends on
Checklist
hard-to-understand areas.
CLA acceptance
_Remove if not applicable.
By submitting the contribution I accept the terms and conditions of the
Contributor License Agreement v1.0
link: https://developers.concordium.com/CLAs/Contributor-License-Agreement-v1.0.pdf
I accept the above linked CLA.